Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Making some improvements #1

Merged
merged 2 commits into from
Aug 5, 2016

Conversation

dignifiedquire
Copy link
Collaborator

Hey, so I discovered this module when I was trying to write my own pull-stream wrapper for net and was very happy, but not everything was working as I hoped. So I started changing and fixing some things as well as I could. I probably made a lot of mistakes so please forgive them.

Currently this adds

  • Tests scripts are running and passing
  • Use the previously commented "direct return" approach in client.js as this worked out very nicely in my tests
  • Enable ipv6 & ipv4 support when dialing and listening
  • Add a close method using pull-abortable to sockets created from handle.js
  • Code style cleanup to use standard (I was a bit confused by the code style so please let me know if you want me to remove this/move to a different code style)

Currently the main issue I'm facing is that when I use the close method it's not closing properly the other side any tips on how to move forward would be appreciated.

A test suite testing these features and showing this problem can be found here: libp2p/js-libp2p-tcp#20 which needs the changes in this PR and afterwards can be run with npm test.

@dominictarr
Copy link
Owner

dominictarr commented Aug 5, 2016

okay normally I dislike standard, but you've added enough that I would consider this is now your module too, so am happy to merge this. I will add you as collaborator and npm owner.

@dominictarr dominictarr merged commit 04a61c2 into dominictarr:master Aug 5, 2016
@dominictarr
Copy link
Owner

merged into 1.0.0

@dominictarr
Copy link
Owner

oh, btw, it looks like this is now good enough to use now?
https://github.com/dominictarr/pull-net#pull-net
so I guess we could move this to pull-stream org

@dignifiedquire dignifiedquire deleted the improvements branch August 5, 2016 22:07
@dignifiedquire
Copy link
Collaborator Author

oh, btw, it looks like this is now good enough to use now?

I think so.
I will also add some more improvements next week that should make it really ready.

@dignifiedquire
Copy link
Collaborator Author

Currently the main issue I'm facing is that when I use the close method it's not closing properly the other side any tips on how to move forward would be appreciated.

Any ideas around why if I use the the new close method in the Handle won't end the streams on both sides?

@ahdinosaur
Copy link

just want to say, this is awesome @dignifiedquire. 👍 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants